[SkyServe][Test] Fix test_smoke.py::test_skyserve_new_autoscaler_update #3824
Conversation
There was a problem hiding this comment.
Sorry for the delay and thanks for catching this issue @landscapepainter! Left two comments : )
| @@ -5,10 +5,10 @@ service: | |||
| replicas: 2 | |||
|
|
|||
| resources: | |||
| cloud: gcp | |||
| cloud: {{generic_cloud}} | |||
There was a problem hiding this comment.
Why do we need this templating stuff considering we have the --cloud in our test already?
There was a problem hiding this comment.
Even though the test yaml with cloud: gcp worked as we expected due to --cloud option, it was confusing and I was not able to come up with a good reason to keep it as cloud: gcp while it is being tested for other clouds as well. Should we keep it as it was without templating?
There was a problem hiding this comment.
Let's keep the original way or just remove the cloud: in the YAML, as it seems unnecessary to do a template when our CLIs are built for being able to override the fields.
There was a problem hiding this comment.
Removed the cloud: from YAML, and reverted the template :)
| ports: 8081 | ||
| cpus: 2+ | ||
|
|
||
| workdir: examples/serve/http_server | ||
|
|
||
| run: python3 server.py | ||
| run: python3 server.py --port 8081 |
There was a problem hiding this comment.
Good catch! Is the other changes in this PR needed except for this one?
There was a problem hiding this comment.
The test will work by only keeping this change.
|
@Michaelvll This is ready for another look! Ran the test again just in case and confirmed it passes. |
Fixing two points:
8081, to run the server script. As the--portoption was not used,examples/serve/http_server/server.pywas using default port value,8080, instead, resulting in error.resource:can specify other clouds than justgcp.Tested (run the relevant ones):
bash format.shpytest tests/test_smoke.pypytest tests/test_smoke.py::test_skyserve_new_autoscaler_updateconda deactivate; bash -i tests/backward_compatibility_tests.sh